Skip to content

Fix added nested rules order#655

Closed
lttb wants to merge 2 commits intomasterfrom
feature/fix-order
Closed

Fix added nested rules order#655
lttb wants to merge 2 commits intomasterfrom
feature/fix-order

Conversation

@lttb
Copy link
Copy Markdown
Member

@lttb lttb commented Dec 22, 2017 •
edited
Loading

Fix an issue from cssinjs/styled-jss#59

The problem is when we add rules by .addRules, it has some issues with nested rules order.

const Div = styled('div')({
  padding: 15,
  '&:hover': {
    '& $button': {
      color: ({primary}) => (primary ? 'red' : 'green'),
    },
  },
})

const Button = styled('button')({
  composes: '$button',
})

const App = (props: {primary?: boolean}) => (
  <div>
    <Div primary={props.primary}>
      <Button>Button</Button>
    </Div>
  </div>
)

expect(getCss(sheet)).toBe(removeWhitespace(sheet.toString()))

image

@lttb lttb force-pushed the feature/fix-order branch from 5b44d27 to 94ffff2 Compare December 22, 2017 02:10
@lttb lttb changed the title Fix nested styles order Fix added nested rules order Dec 22, 2017
@lttb lttb requested a review from kof December 22, 2017 06:29
kof
kof reviewed Dec 22, 2017
View reviewed changes
Comment thread tests/integration/plugins.js
})
})

describe('jss-nested', () => {
Copy link
Copy Markdown
Member

@kof kof Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test actually belongs to jss-nested

Copy link
Copy Markdown
Member

@kof kof Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively we could describe the same scenario using plugins api instead of jss-nested plugin

Copy link
Copy Markdown
Member Author

@lttb lttb Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this test should be here because it tests the way how rules apply in some cases, so the same scenario instead of jss-nested looks better for me

kof
kof reviewed Dec 22, 2017
View reviewed changes
Comment thread tests/integration/plugins.js
})

it('should save the added nested rules order', () => {
expect(getCssFromSheet(sheet)).to.be(removeWhitespace(sheet.toString()))
Copy link
Copy Markdown
Member

@kof kof Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we only use DOM based tests in functional tests, is DOM really needed here or sheet.toString() + comparing with result would be enough?

Copy link
Copy Markdown
Member Author

@lttb lttb Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I think we can replace expect with the exact result

kof
kof reviewed Dec 22, 2017
View reviewed changes
Comment thread src/StyleSheet.js
// Don't insert rule directly if there is no stringified version yet.
// It will be inserted all together when .attach is called.
if (queue) queue.push(rule)
if (queue) queue.unshift(rule)
Copy link
Copy Markdown
Member

@kof kof Dec 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure why this is not breaking anything else. You are reverting the order of added rules for all addRule calls. Maybe there is a missing test which ensures the right order.

We have actually an options.index which is set in jss-nested in order to have the control over the order. Maybe jss-nested sets the wrong index?

@kof
Copy link
Copy Markdown
Member

kof commented Jan 25, 2018

totally forgot this one

@kof
Copy link
Copy Markdown
Member

kof commented Jan 25, 2018

@lttb whats the state here?

@kof kof closed this Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kof kof kof left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@lttb @kof
⚡